Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All of my changes rebased on dev #919

Closed
wants to merge 26 commits into from

Conversation

NightMachinery
Copy link
Contributor

I rebased my all of my changes on top of the development branch. Some of them became moot, e.g., #911 is no longer needed (probably because of QT 6).

I can cherry-pick these and send individual PRs if you like.

I have added an env var to the build process, SIOYEK_NIGHT_P, which defines NIGHT_P. This allows me to put some (small) parts of the changes that are not beneficial for everyone under #ifdef NIGHT_P. If I send individual PRs, I can just not pick the commits that add these. But rebasing the commits somewhat reduced their coherency (I had to manually apply some of the later changes earlier during the conflict resolution process), so if you can just accept them it would be much easier. They can also serve as an example for other people on how they can customize the app for their own needs while keeping the custom changes isolated.

The PRs I have sent previously are also included in this one, but now they are mergeable with the dev branch:

Other additions:
Config:

  • keyboard_select_copy_p: allows automatic copying of all keyboard selections
  • regex_searching: another mode instead of fuzzy_searching; allows using regular expressions to select from menus. pat1 pat2 ... is split on whitespace and becomes pat1 AND pat2. This is a minimal implementation of ugrep --bool. This mode does NOT sort, it only filters.
  • scroll_zoom_inc_factor: allows setting a separate zoom factor for the mouse

New Commands:

  • PushStateCommand: Pushes the current state in the navigation history
  • next_page_smart: Go to next page (tries to go to the next column on two-column pages)
  • previous_page_smart: Go to previous page (tries to go to the previous column on two-column pages)

Other Changes:

  • shift+scrolling works even with horizontal lock
  • Some commits on main were not previously merged into development, they have also been picked up on by this PR.

@ahrm
Copy link
Owner

ahrm commented Jan 16, 2024

Whoa that's a lot of changes, thanks :). I will have to test and review them when I have the time.

I don't think I can accept #ifdef NIGHT_P though because it can make the code more confusion and harder to understand. You can imagine if everyone wanted to define their own parts of the code it would get extremely complicated very fast.

@NightMachinery
Copy link
Contributor Author

Thanks. I can filter out the commits that introduce NIGHT_P, if the other parts are okay.

The macOS build instructions also need an update for the new QT 6, which I haven't committed yet.

@ahrm
Copy link
Owner

ahrm commented Jan 17, 2024

I have looked at some of the code and I do have some other notes. I will post the full comments when I have reviewed all the code (probably in an hour or two).

@ahrm
Copy link
Owner

ahrm commented Jan 17, 2024

Some notes:

  1. Some boolean functions like previous_page_smart don't return values on all execution paths.
  2. A better name for next_page_smart might be next_column? Because it doesn't really go to the next page. For example if executed at the top of the first column it just jumps to the bottom of the page. Also I was experimenting with adding some basic scripting using javascript to sioyek. So while I don't think it is currently possible to do this using scripting, I would rather add the functionality so that the users themselves can define a similar functionality to next_page_smart (rather than adding a separate command).
  3. The new version of sioyek has a much better macro and scripting support. So the functionality of keyboard_select_p can be implemented by adding this to keys_user.config: keyboard_select;copy <keybind>
    This was impossible in previous versions of sioyek because copy would be executed immediately after the keybind is pressed. But in the newer versions of sioyek we correctly wait for keyboard_select to be done before executing copy. So there is no need for a separate config to add this functionality.
  4. Some names don't follow snake-case naming convention. e.g.: isKeyPressed, keyStates, filterString.
  5. I think isKeyPressed is only used in #ifdef NIGHT_P so it can be removed.
  6. Regarding the changes to delete key handling and should_trigger_delete: I am not very familiar witth macos so I may be wrong but what happens when the user pressed the backspace key to delete the text entered e.g. in bookmarks search bar? Doesn't that trigger should_trigger_delete and delete the bookmark instead of deleting the search prompt?

@NightMachinery
Copy link
Contributor Author

Thanks.

  • Indeed, using backspace for deleting stuff will make it unusable for deleting chars. I hadn't thought of that. Alt+Backspace will erase the current word completely, which is what I usually use I guess. I don't know what should be done here. Should I set shift+backspace for deleting an entry on macOS?
    Also, after the rebase on dev, backspace doesn't trigger delete at all. I should debug that.

  • isKeyPressed: Indeed, I can also remove this part.
    The ideal solution here is (IMO) to allow the user to configure the set of keys they want to trigger zoom-scrolling or horizontal-scrolling. (This would need isKeyPressed.) But I don't know how that can fit into the current config system. The reason I needed this badly was because I use sticky keys, which makes modifier keys "stick around" after pressing them unless you press another key. This behavior doesn't work well with scrolling, as scrolling doesn't trigger the release of the modifier key. So I needed to use some non-modifier key.

  • Yes, I should rename next_page_smart to next_column_smart. I like the _smart postfix, as it shows the command is a best-effort heuristic.
    Adding scripting is great (I suggest Lua, I have had a good experience using it to script Hammerspoon and MPV), but next_page_smart needs other commands to return whether they moved the screen substantially or not. So it would not be implementable without changes to the core C++ code as of now.

I am a bit busy now, I'll send a revised PR later.

@NightMachinery
Copy link
Contributor Author

I fixed the problem with backspace not working after the rebase and pushed it. After testing it, backspace works fine for deleting chars (as it is triggered on key press, while the deletion check happens on key release). As long as the user has not selected an item using the arrow keys, backspace can't trigger a delete anyhow.

Perhaps we can just disable backspace for character deleting when any item is selected. I don't know how useful that ability is.

@ahrm
Copy link
Owner

ahrm commented Jan 18, 2024

I like your suggestion of using a shortcut like shift+backspace to delete in MacOS better because the current approach has a high probability of a user accidentally deleting their data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants